MLE-24685 Trying fixes for failing tests #981
Conversation
rjrudin
requested review from
BillFarber,
anu3990 and
stevebio
as code owners
There was a problem hiding this comment.
Pull Request Overview
This PR fixes failing tests by simplifying the nodejs-optic-nodes test to remove XML document testing and addressing "done called multiple times" errors in two other tests.
- Simplified nodejs-optic-nodes test by removing XML document construction and related assertions
- Fixed "done called multiple times" error in nodejs-dmsdk-removeAllUris test by adding error handling
- Fixed "done called multiple times" error in nodejs-dmsdk-queryToTransformAll test by removing done() call from onBatchSuccess callback
Reviewed Changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| test-complete/nodejs-optic-nodes.js | Removed XML document testing code and related assertions to simplify test |
| test-complete/nodejs-dmsdk-removeAllUris.js | Added error handling to prevent multiple done() calls |
| test-complete/nodejs-dmsdk-queryToTransformAll.js | Removed done() call from onBatchSuccess to prevent multiple calls |
| CONTRIBUTING.md | Added documentation about test-complete folder tests |
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
| console.log(JSON.stringify(output, null, 2)); | ||
| expect(output.columns[1].name).to.equal('myJSON'); | ||
| expect(output.columns[1].type).to.equal('object'); |
There was a problem hiding this comment.
Debug console.log statement should be removed before merging to production code.
| console.log(JSON.stringify(output, null, 2)); | |
| expect(output.columns[1].name).to.equal('myJSON'); | |
| expect(output.columns[1].type).to.equal('object'); | |
| expect(output.columns[1].name).to.equal('myJSON'); | |
| expect(output.columns[1].type).to.equal('object'); | |
| expect(output.columns[1].type).to.equal('object'); |
|
Copyright Validation Results ⏭️ Skipped (Excluded) Files
✅ Valid Files
✅ All files have valid copyright headers! |
| progress.docsFailedToBeTransformed.should.be.equal(0); | ||
| progress.timeElapsed.should.be.greaterThanOrEqual(0); | ||
| }), | ||
| onCompletion: ((summary) => { |
There was a problem hiding this comment.
done is called here only in case of exception. The purpose of adding a try block is so that whenever something goes wrong here, it does not show timeout.
There was a problem hiding this comment.
This is in regards to this intermittent error:
Copilot's reasoning was that "done()" would be called multiple times if assertions failed in 2 or more batches. Instead, the assertion errors should bubble up automatically , and onCompletion will handle calling "done()".
| }); | ||
| }).catch(done); | ||
|
|
||
| }); |
There was a problem hiding this comment.
We don't need to catch done. I know there are a lot of tests with this statement but I don't see much use.
There was a problem hiding this comment.
Copilot added it - here's it's reasoning:
Purpose of .catch(done) on line 47
The .catch(done) serves as a safety net for the beforeEach hook to handle errors that occur in the writeAll() method itself (not just the write operation).
Two-layer error handling:
- Primary:
onCompletioncallback handles errors during the write operation (viasummary.error) - Secondary:
.catch(done)handles errors from thewriteAll()method itself
Without .catch(done), these scenarios would cause the test to hang:
- MarkLogic server connection failures
- Authentication errors
- Network timeouts
- Malformed requests that throw immediately
This prevents the "done() called multiple times" issue by ensuring:
- Only one error path calls
done() - Proper test failure instead of indefinite hanging
- Robust error handling for Jenkins environment differences
This is a standard pattern for async Mocha tests - always include .catch(done) to handle promise rejections outside of your main success/error logic.
rjrudin
force-pushed
the
feature/construct-json-test
branch
from
f3e992a to
898d70e
Compare
Simplifying nodejs-optic-nodes - it doesn't need to do XML documents, we have other tests for that. Hoping this avoids the mysterious 500 error. Then had Copilot fix the other two tests where they were failing with a message of "done called multiple times". Adding 500 response body to error message
rjrudin
force-pushed
the
feature/construct-json-test
branch
from
898d70e to
85a29a0
Compare
And undoing a couple test changes that didn't seem to help.
rjrudin
force-pushed
the
feature/construct-json-test
branch
from
85a29a0 to
4c2a846
Compare
|
Closing this. Going to simplify it down with a change to responder to include the request body for a 500. |
Simplifying nodejs-optic-nodes - it doesn't need to do XML documents, we have other tests for that. Hoping this avoids the mysterious 500 error.
Then had Copilot fix the other two tests where they were failing with a message of "done called multiple times".